-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Template for unit tests #187
Conversation
@aulemahal @Zeitsperre Feel free to work directly on this branch and to change/remove what I've done. Once we're happy with the formatting, we'll merge it and I'll start making real unit tests. |
# Conflicts: # environment-dev.yml
What I'm uploading here is a version that uses |
Deux questionnements spécifiques:
|
for more information, see https://pre-commit.ci
@RondeauG
|
This should be good for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will wait on @aulemahal's approval as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment to the changes in climatological_mean
.
For the test, I think this is a good example of what we want. Even, it is on the exhaustive side, which is good.
Warning : all tests here are done on artificial data, and thus it may fail to tests some real-life caveats. When adding more tests, we could make use of xclim's test data too.
xscen/aggregate.py
Outdated
|
||
if to_level is not None: | ||
ds_rolling.attrs["cat:processing_level"] = to_level | ||
ds_rolling.attrs["cat:processing_level"] = to_level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't fit the docstring, which says None is possible!
I'm guessing you want to change the docstring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, true! I got confused because the default is a string, not None
. I'll reverse that.
Pull Request Checklist:
number
) and pull request (:pull:number
) has been addedWhat kind of change does this PR introduce?
climatological_mean
andcompute_deltas
when using daily data.Does this PR introduce a breaking change?
Other information:
This is a start for #9